Skip to content

Rust: New query rust/access-after-lifetime-ended #19702

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 32 commits into
base: main
Choose a base branch
from

Conversation

geoffw0
Copy link
Contributor

@geoffw0 geoffw0 commented Jun 9, 2025

New query rust/access-after-lifetime-ended, for detecting pointer dereferences after the lifetime of the pointed-to object has ended. Makes use of some existing tests that were created for rust/access-invalid-pointer (before I realized that the idea for that query needed breaking into two separate queries). Also adds quite a lot of new test cases as well.

Note that the query is currently @precision medium due to several remaining false positive results in the tests (and in MRVA results). I made some effort to fix these, but I didn't get them all, I feel it's time to get what we have merged and plan further improvements as follow-up work.

Before merging:

  • QL review
  • docs review
  • DCA run

@Copilot Copilot AI review requested due to automatic review settings June 9, 2025 17:41
@geoffw0 geoffw0 requested a review from a team as a code owner June 9, 2025 17:41
@geoffw0 geoffw0 added the Rust Pull requests that update Rust code label Jun 9, 2025
Copy link
Contributor

github-actions bot commented Jun 9, 2025

QHelp previews:

rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.qhelp

Access of a pointer after its lifetime has ended

Dereferencing a pointer after the lifetime of its target has ended causes undefined behavior. Memory may be corrupted, causing the program to crash or behave incorrectly, in some cases exposing the program to potential attacks.

Recommendation

When dereferencing a pointer in unsafe code, take care that the pointer is still valid at the time it is dereferenced. Code may need to be rearranged or changed to extend lifetimes. If possible, rewrite the code using safe Rust types to avoid this kind of problem altogether.

Example

In the following example, val is local to get_pointer so its lifetime ends when that function returns. However, a pointer to val is returned and dereferenced after that lifetime has ended, causing undefined behavior:

fn get_pointer() -> *const i64 {
	let val = 123;

	&val
} // lifetime of `val` ends here, the pointer becomes dangling

fn example() {
	let ptr = get_pointer();
	let val;

	// ...

	unsafe {
		val = *ptr; // BAD: dereferences `ptr` after the lifetime of `val` has ended
	}

	// ...
}

One way to fix this is to change the return type of the function from a pointer to a Box, which ensures that the value it points to remains on the heap for the lifetime of the Box itself. Note that there is no longer a need for an unsafe block as the code no longer handles pointers directly:

fn get_box() -> Box<i64> {
	let val = 123;

	Box::new(val) // copies `val` onto the heap, where it remains for the lifetime of the `Box`.
}

fn example() {
	let ptr = get_box();
	let val;

	// ...

	val = *ptr; // GOOD

	// ...
}

References

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a new CodeQL query to detect pointer dereferences after the lifetime of their target has ended, including examples, documentation, and scope‐tracking support.

  • Introduce AccessAfterLifetime.ql with configuration, flow definition, and filtering logic
  • Add QL test harness entries (.qlref), examples (Good.rs/Bad.rs), and documentation (.qhelp)
  • Extend the internal QL library to track enclosing blocks (getEnclosingBlock) and support the new query

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
rust/ql/test/query-tests/security/CWE-825/options.yml Add futures dependency for async tests
rust/ql/test/query-tests/security/CWE-825/main.rs Invoke newly added test cases in main()
rust/ql/test/query-tests/security/CWE-825/deallocation.rs Update existing invalid-pointer test annotations
rust/ql/test/query-tests/security/CWE-825/AccessAfterLifetime.qlref Register the new query with test harness
rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeGood.rs Add good example using Box
rust/ql/src/queries/security/CWE-825/AccessAfterLifetimeBad.rs Add bad example that returns a dangling pointer
rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql Implement the new query logic
rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.qhelp Add documentation and examples
rust/ql/lib/codeql/rust/security/AccessAfterLifetimeExtensions.qll Define sources, sinks, barriers, and scope checks
rust/ql/lib/codeql/rust/elements/internal/VariableImpl.qll Add Variable.getEnclosingBlock()
rust/ql/lib/codeql/rust/elements/internal/AstNodeImpl.qll Add AstNode.getEnclosingBlock()
Comments suppressed due to low confidence (1)

rust/ql/src/queries/security/CWE-825/AccessAfterLifetime.ql:44

  • The predicate isFromMacroExpansion() does not exist; you likely meant isInMacroExpansion() to filter out macro expansions.
not sinkNode.getNode().asExpr().getExpr().isFromMacroExpansion()

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 11, 2025

DCA:

  • 95 results for the new query; that's quite a lot...
    • confusing results that have sources in macros - fixed ✅
    • results matching self; I think this kind of thing is probably OK: match self { EnumValue(x) => &x.y }
      • but the query is @precision medium, so deferring that for now should be OK I think
  • otherwise LGTM

@geoffw0 geoffw0 added no-change-note-required This PR does not need a change note ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. labels Jun 12, 2025
mchammer01
mchammer01 previously approved these changes Jun 13, 2025
Copy link
Contributor

@mchammer01 mchammer01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geoffw0 - LGTM ✨
Added a couple of very minor comments/suggestions.

@geoffw0
Copy link
Contributor Author

geoffw0 commented Jun 13, 2025

Thanks for the review @mchammer01 , I've accepted both suggestions. :)

Copy link
Contributor

@paldepind paldepind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really great to me! Lots of true positives.


use_the_stack();

let v3 = *r1; // $ SPURIOUS: Alert[rust/access-after-lifetime-ended]=v2_value
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An idea for getting rid of this false positive could be to change DereferenceSink such that only dereferences inside unsafe blocks are considered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it's a bit of a shortcut because it's not the real reason these cases are safe - but it might eliminate some FPs yielding better overall results in practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just implemented and pushed this - the approach works pretty well in practice.

// ...

unsafe {
val = *ptr; // BAD: dereferences `ptr` after the lifetime of `val` has ended
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit confusing that there is two vals here and the lifetime of the val 5 lines up has not ended. Just renaming one one them would help I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No this is the same val, we're assigning to it not creating another val.

It's a different val from the one 12 lines up however - is using the same name inside get_pointer confusing or helpful?

Co-authored-by: Simon Friis Vindum <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation no-change-note-required This PR does not need a change note ready-for-doc-review This PR requires and is ready for review from the GitHub docs team. Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants